Recover module added - #101
Conversation
WalkthroughAdds two exercises to the catalog: 36_epoch and 37_recover. Introduces template and tests for 37_recover, plus a reference solution. Updates epoch template test file with an incomplete function. No other files modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Run as recover_exercise.Run
participant DoWork as recover_exercise.DoWork
Tester->>Run: Run(n)
Note over Run: defer func(){ v = recover() }()
Run->>DoWork: DoWork(n)
alt n < 0
DoWork-->>DoWork: panic("input cannot be negative")
Note over Run: deferred recover() captures panic
Run-->>Tester: recoveredValue = "input cannot be negative"
else n ≥ 0
DoWork-->>Run: return
Run-->>Tester: recoveredValue = nil
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/exercises/solutions/36_epoch/epoch.go (1)
57-69: Simplify the UnixSeconds map entry.Lines 60-61 create a redundant string with duplicate formatted times. The expression
time.Unix(now.Unix(), 0).Format(...)on line 60 produces the same result astime.Unix(now.Unix(), 0).UTC().Format(...)on line 61 (sincetime.Unixalready returns UTC time). This concatenation pattern is unclear and doesn't add value.Consider simplifying to:
return map[string]string{ - "UnixSeconds": time.Unix(now.Unix(), 0).Format("2006-01-02 15:04:05") + " (epoch: " + - time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05") + ")", + "UnixSeconds": time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05"), "UnixSecondsRaw": time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05"),Or clarify the intended distinction between "UnixSeconds" and "UnixSecondsRaw".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/36_epoch/epoch.go(1 hunks)internal/exercises/solutions/37_recover/recover.go(1 hunks)internal/exercises/templates/36_epoch/epoch.go(1 hunks)internal/exercises/templates/36_epoch/epoch_test.go(1 hunks)internal/exercises/templates/37_recover/recover.go(1 hunks)internal/exercises/templates/37_recover/recover_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/exercises/templates/37_recover/recover_test.go (2)
internal/exercises/solutions/37_recover/recover.go (1)
Run(14-30)internal/exercises/templates/37_recover/recover.go (1)
Run(15-25)
internal/exercises/solutions/37_recover/recover.go (1)
internal/exercises/templates/37_recover/recover.go (2)
DoWork(4-10)Run(15-25)
internal/exercises/templates/36_epoch/epoch_test.go (1)
internal/exercises/templates/36_epoch/epoch.go (2)
EpochToTime(6-9)TimeToEpoch(12-15)
internal/exercises/templates/37_recover/recover.go (1)
internal/exercises/solutions/37_recover/recover.go (2)
DoWork(4-10)Run(14-30)
🔇 Additional comments (13)
internal/exercises/templates/37_recover/recover_test.go (2)
7-13: LGTM!The test correctly verifies that
Run(10)does not panic and returnsnil, validating the non-panic code path.
15-28: LGTM!The test properly verifies panic recovery behavior, including type assertion with safety checks and validation of the expected panic message.
internal/exercises/templates/37_recover/recover.go (2)
3-10: LGTM!The
DoWorkfunction correctly demonstrates a panic scenario with a clear error message. The implementation aligns with test expectations and the solution.
12-25: LGTM!The template structure is well-designed for teaching defer/recover patterns. The named return value and instructional comments provide clear guidance for learners to implement the recovery logic.
internal/exercises/templates/36_epoch/epoch.go (1)
3-15: LGTM!The template functions are well-documented with clear format specifications. The intentionally incorrect implementations serve the exercise's learning objectives effectively.
internal/exercises/solutions/37_recover/recover.go (2)
3-10: LGTM!The
DoWorkimplementation is correct and consistent with the template and test expectations.
12-30: LGTM!The defer/recover implementation is textbook-correct and demonstrates proper panic recovery patterns. The educational comments effectively explain each step of the recovery mechanism.
internal/exercises/templates/36_epoch/epoch_test.go (1)
5-20: Approve symmetric epoch/time conversion tests. Epoch timestamp 1633046400 correctly maps to 2021-10-01 00:00:00 UTC.internal/exercises/catalog.yaml (1)
180-188: LGTM! Well-structured epoch exercise entry.The catalog entry is well-defined with appropriate difficulty, topics, and helpful hints that guide learners through epoch conversion concepts.
internal/exercises/solutions/36_epoch/epoch.go (4)
10-22: LGTM! Current time getters are correctly implemented.All three functions properly use
time.Now()and the appropriate conversion methods. The millisecond conversion correctly dividesUnixNano()by the millisecond duration constant.
26-34: LGTM! Formatted time functions are correct.Both functions properly format the current UTC time with appropriate precision levels (microsecond and second).
40-43: LGTM! Clean epoch-to-time conversion.The function correctly uses
time.Unix()for conversion and formats the result in UTC.
47-53: Verify error handling for parse failures.The function silently returns
0when parsing fails. For an educational exercise solution, this might confuse learners who pass invalid input. Consider whether returning an error or panicking would better demonstrate proper error handling patterns.However, if this silent failure is intentional for the exercise design (e.g., tests expect
0for invalid input), then the current implementation is acceptable.Do you want the solution to demonstrate explicit error handling, or is returning
0the intended behavior for this exercise?
| func formatInt(val int64) string { | ||
| return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05") | ||
| } No newline at end of file |
There was a problem hiding this comment.
Critical: formatInt incorrectly interprets input.
The formatInt helper calls time.Unix(0, val) which interprets val as nanoseconds (the second parameter). However, this function is called with:
now.Unix()(line 63) - which is in secondsnow.UnixMilli()(line 64) - which is in millisecondsnow.UnixNano()(line 65) - which is in nanoseconds (only this is correct)
This will produce incorrect dates for all callers except UnixNanoseconds.
The correct implementation should be:
-// Helper: format int64 as string
-func formatInt(val int64) string {
- return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05")
-}
+// Helper: format unix timestamp (seconds) as string
+func formatSeconds(val int64) string {
+ return time.Unix(val, 0).UTC().Format("2006-01-02 15:04:05")
+}
+
+// Helper: format unix timestamp (milliseconds) as string
+func formatMillis(val int64) string {
+ return time.Unix(0, val*int64(time.Millisecond)).UTC().Format("2006-01-02 15:04:05")
+}
+
+// Helper: format unix timestamp (nanoseconds) as string
+func formatNanos(val int64) string {
+ return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05")
+}And update the calls in NowFormats():
return map[string]string{
"UnixSeconds": time.Unix(now.Unix(), 0).Format("2006-01-02 15:04:05") + " (epoch: " +
time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05") + ")",
"UnixSecondsRaw": time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05"),
- "UnixSecondsInt": formatInt(now.Unix()),
- "UnixMilliseconds": formatInt(now.UnixMilli()),
- "UnixNanoseconds": formatInt(now.UnixNano()),
+ "UnixSecondsInt": formatSeconds(now.Unix()),
+ "UnixMilliseconds": formatMillis(now.UnixMilli()),
+ "UnixNanoseconds": formatNanos(now.UnixNano()),
"FormattedFull": GetCurrentFormattedTime(),
"FormattedSimple": GetCurrentFormattedTimeSimple(),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func formatInt(val int64) string { | |
| return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05") | |
| } | |
| // --- Replace the old formatInt helper with three explicit functions --- | |
| -// Helper: format int64 as string | |
| -func formatInt(val int64) string { | |
| - return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05") | |
| -} | |
| // Helper: format unix timestamp (seconds) as string | |
| func formatSeconds(val int64) string { | |
| return time.Unix(val, 0).UTC().Format("2006-01-02 15:04:05") | |
| } | |
| // Helper: format unix timestamp (milliseconds) as string | |
| func formatMillis(val int64) string { | |
| // time.Millisecond is a time.Duration (in nanoseconds) | |
| return time.Unix(0, val*int64(time.Millisecond)).UTC().Format("2006-01-02 15:04:05") | |
| } | |
| // Helper: format unix timestamp (nanoseconds) as string | |
| func formatNanos(val int64) string { | |
| return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05") | |
| } | |
| // --- And in NowFormats(), update the calls: --- | |
| return map[string]string{ | |
| "UnixSeconds": time.Unix(now.Unix(), 0).Format("2006-01-02 15:04:05") + | |
| " (epoch: " + time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05") + ")", | |
| "UnixSecondsRaw": time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05"), | |
| - "UnixSecondsInt": formatInt(now.Unix()), | |
| - "UnixMilliseconds": formatInt(now.UnixMilli()), | |
| "UnixSecondsInt": formatSeconds(now.Unix()), | |
| "UnixMilliseconds": formatMillis(now.UnixMilli()), | |
| "UnixNanoseconds": formatNanos(now.UnixNano()), | |
| "FormattedFull": GetCurrentFormattedTime(), | |
| "FormattedSimple": GetCurrentFormattedTimeSimple(), | |
| } |
🤖 Prompt for AI Agents
internal/exercises/solutions/36_epoch/epoch.go lines 72-74: formatInt currently
treats its int64 input as nanoseconds (time.Unix(0, val)) which is wrong for
callers passing seconds and milliseconds; change the implementation to convert
the input correctly based on units (use time.Unix(val, 0) for seconds,
time.Unix(val/1e3, (val%1e3)*1e6) for milliseconds, and time.Unix(0, val) for
nanoseconds) and update NowFormats() calls so each call passes the correct form
(or call separate helper functions) — ensure the logged/returned formatted time
uses the corrected conversion and UTC formatting.
|
can u check once now |
|
@Omesh2004 : Can you resolve the conflicts in the mean time please? |
zhravan
left a comment
zhravan
left a comment
There was a problem hiding this comment.
@Omesh2004 : Can you take latest pull and resolve the conflicts and raise final PR? Thank you so much for the efforts!
2 participants
Summary
Describe the change and its motivation.
i have added excercise related to recover exercise template
Related issues
Fixes #
Summary by CodeRabbit
New Features
Tests